chore: add joey wallet, and other missing wallets to applications doc#3082
chore: add joey wallet, and other missing wallets to applications doc#3082Patel-Raj11 merged 2 commits intoXRPLF:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughUpdated the "Wallets and wallet tools" section in APPLICATIONS.md by adding entries for Joey Wallet and Crossmark Wallet while removing the GemWallet entry. Net addition of 8 lines to the documentation. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
APPLICATIONS.md (2)
83-83: Prefer descriptive link text over raw URLs.Improves readability and accessibility.
Apply this diff:
- Joey Wallet is a secure, self-custody cryptocurrency wallet and gateway to Web3 decentralized applications (dApps) on the XRP Ledger (XRPL). Documentation and integration details are located at [https://docs.joeywallet.xyz/](https://docs.joeywallet.xyz). + Joey Wallet is a secure, self-custody cryptocurrency wallet and gateway to Web3 decentralized applications (dApps) on the XRP Ledger (XRPL). Documentation: [Joey Wallet Docs](https://docs.joeywallet.xyz). - Crossmark is a browser-first, self-custodial wallet built for interacting with the XRP Ledger. Send, receive, create value with ease. Documentation can be found at [https://docs.crossmark.io/](https://docs.crossmark.io/). + Crossmark is a browser-first, self-custodial wallet for interacting with the XRP Ledger. Documentation: [Crossmark Docs](https://docs.crossmark.io/).Also applies to: 87-87
81-88: Optional: consider alphabetical ordering within the Wallets list.If the section intends alphabetical order, place “Crossmark” and “Joey Wallet” accordingly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
APPLICATIONS.md(1 hunks)
🔇 Additional comments (2)
APPLICATIONS.md (2)
81-88: Additions look good.Joey Wallet and Crossmark entries are clear and match the section’s formatting.
81-88: Confirm inclusion criteria (uses xrpl.js) — Crossmark verified; Joey Wallet unverified
- Crossmark — docs explicitly expose a lightweight xrpl.js on window.xrpl and include usage examples (keep).
- Joey Wallet — public developer docs reference WalletConnect / wc-client; no xrpl.js references found. Provide a source showing Joey Wallet uses xrpl.js or remove/flag the Joey Wallet entry in APPLICATIONS.md (lines 81–88).
|
@interc0der Hi, All the commits must be signed for this PR to get merged. Can you sign the commits and then we are good to merge. |
208b0da to
eb46241
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/ripple-keypairs/src/index.ts (1)
98-109: Consider using existinggetSigningSchemehelper to simplify.The function duplicates the algorithm-to-scheme mapping that already exists in
getSigningScheme(lines 23-26). This would also eliminate the unreachable throw statement.♻️ Proposed simplification
function derivePublicKey(privateKey: string): string { const algorithm = getAlgorithmFromPrivateKey(privateKey) - - if (algorithm === 'ecdsa-secp256k1') { - return secp256k1.deriveKeypairFromPrivateKey(privateKey).publicKey - } - // eslint-disable-next-line `@typescript-eslint/no-unnecessary-condition` - if (algorithm === 'ed25519') { - return ed25519.deriveKeypairFromPrivateKey(privateKey).publicKey - } - throw new Error('Unknown signing scheme algorithm') + return getSigningScheme(algorithm).deriveKeypairFromPrivateKey(privateKey).publicKey }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ripple-keypairs/src/index.ts` around lines 98 - 109, derivePublicKey duplicates algorithm detection; replace its use of getAlgorithmFromPrivateKey with the existing getSigningScheme(privateKey) helper, then branch on the returned signing scheme (e.g., 'ecdsa-secp256k1' vs 'ed25519') to call secp256k1.deriveKeypairFromPrivateKey(...) or ed25519.deriveKeypairFromPrivateKey(...) and return .publicKey; this reuses getSigningScheme, removes duplicated mapping and makes the final throw unreachable so it can be eliminated or reduced to a defensive assertion if desired.packages/xrpl/test/wallet/index.test.ts (4)
440-440: Inconsistent naming: usefromPrivateKeywithout space.Other test suites use method names directly:
fromSeed,fromSecret,fromMnemonic,fromEntropy. This should befromPrivateKey(no space) for consistency.♻️ Suggested fix
- describe('from PrivateKey', function () { + describe('fromPrivateKey', function () {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/wallet/index.test.ts` at line 440, The test suite title is using "from PrivateKey" with a space but should be consistent with other suites; update the describe block that currently reads "from PrivateKey" to "fromPrivateKey" so it matches the method naming style used by other tests (e.g., fromSeed, fromSecret, fromMnemonic, fromEntropy) and ensures consistency with the tested function name fromPrivateKey.
442-448: Use camelCase for variable names per JavaScript/TypeScript conventions.Variable names like
mockWallet_secp256k1andmockWallet_ed25519use snake_case which is inconsistent with the rest of the codebase.♻️ Suggested fix
- const mockWallet_secp256k1 = { + const mockWalletSecp256k1 = { address: 'rhvh5SrgBL5V8oeV9EpDuVszeJSSCEkbPc', ... }- const mockWallet_ed25519 = { + const mockWalletEd25519 = { address: 'rszPLM97iS8mFTndKQNexGhY1N9ionLVAx', ... }Also applies to: 464-470
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/wallet/index.test.ts` around lines 442 - 448, Rename the snake_case test variables to camelCase (e.g., change mockWallet_secp256k1 -> mockWalletSecp256k1 and mockWallet_ed25519 -> mockWalletEd25519) in the test file; update every reference to these identifiers in tests (constructors, assertions, imports/exports within the file) so the new names are used consistently, keeping the object shapes and values unchanged and ensuring no leftover snake_case identifiers remain.
440-483: Consider adding test for empty string input.The
fromPrivateKeyimplementation validates thatprivateKeyis a non-empty string and throwsValidationError. Adding a test for this case would improve coverage:💚 Suggested test addition
it('throws ValidationError for empty string', function () { assert.throws( () => Wallet.fromPrivateKey(''), /privateKey must be a non-empty string/u, ) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/wallet/index.test.ts` around lines 440 - 483, Add a test under the "from PrivateKey" suite (near the secp256k1/ed25519 tests) that calls Wallet.fromPrivateKey with an empty string and asserts it throws the expected ValidationError; specifically, add an it block that calls Wallet.fromPrivateKey('') and asserts the thrown error matches /privateKey must be a non-empty string/ (reference: Wallet.fromPrivateKey and the existing test cases in this describe block).
456-460: Consider asserting specific error type or message in error tests.The current tests only verify that an error is thrown, but don't validate the error type or message. This could mask unexpected errors.
♻️ Suggested enhancement
it('throws error for malformed secp256k1 private key', function () { - assert.throws(() => - Wallet.fromPrivateKey(mockWallet_secp256k1.privateKey.slice(0, 10)), - ) + assert.throws( + () => Wallet.fromPrivateKey(mockWalletSecp256k1.privateKey.slice(0, 10)), + /Invalid private key/u, + ) })Also applies to: 477-481
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/wallet/index.test.ts` around lines 456 - 460, The tests that call Wallet.fromPrivateKey with malformed secp256k1 input should assert the specific error type or message rather than only that something was thrown; update the failing assertions (the test invoking Wallet.fromPrivateKey(mockWallet_secp256k1.privateKey.slice(0, 10)) and the similar case around lines 477-481) to pass a second argument to assert.throws that checks either an error constructor or a RegExp for the expected message (e.g., assert.throws(() => Wallet.fromPrivateKey(...), { name: 'Error', message: /malformed|invalid private key/i }) or assert.throws(() => Wallet.fromPrivateKey(...), /invalid private key/)) so the tests validate the exact failure mode from Wallet.fromPrivateKey.packages/xrpl/src/Wallet/index.ts (1)
177-190: Consider supportingmasterAddressoption for Regular Key Pair use cases.Other factory methods like
fromSeed,fromEntropy, andfromMnemonicsupport amasterAddressoption for Regular Key Pair scenarios. For API consistency, consider adding this option:♻️ Suggested enhancement
/** * Derives a wallet from a private key. * * `@param` privateKey - A string used to generate a keypair (publicKey/privateKey) to derive a wallet. + * `@param` opts - (Optional) Options to derive a Wallet. + * `@param` opts.masterAddress - Include if a Wallet uses a Regular Key Pair. It must be the master address of the account. * `@returns` A Wallet derived from a private key. * * `@throws` ValidationError if private key is not a valid string */ - public static fromPrivateKey(privateKey: string): Wallet { + public static fromPrivateKey( + privateKey: string, + opts: { masterAddress?: string } = {}, + ): Wallet { if (!privateKey || typeof privateKey !== 'string') { throw new ValidationError('privateKey must be a non-empty string') } - return new Wallet(derivePublicKey(privateKey), privateKey) + return new Wallet(derivePublicKey(privateKey), privateKey, { + masterAddress: opts.masterAddress, + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/Wallet/index.ts` around lines 177 - 190, Add an optional options parameter to fromPrivateKey to accept a masterAddress for Regular Key Pair scenarios: change signature to fromPrivateKey(privateKey: string, options?: { masterAddress?: string }): Wallet, validate that options.masterAddress (if provided) is a non-empty string (throw ValidationError otherwise), and pass it into the Wallet constructor (i.e., new Wallet(derivePublicKey(privateKey), privateKey, options?.masterAddress)) so behavior matches fromSeed/fromEntropy/fromMnemonic.packages/ripple-keypairs/src/signing-schemes/ed25519/index.ts (1)
37-37: Consider usinghexToBytesfor consistency with the codebase.The codebase imports
hexToBytesfrom@xrplf/isomorphic/utilsin other modules (e.g.,packages/ripple-keypairs/src/index.ts). UsingBuffer.from(normalizedPrivateKey, 'hex')works but is inconsistent with the browser-compatible utility pattern used elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ripple-keypairs/src/signing-schemes/ed25519/index.ts` at line 37, Replace the direct Buffer.from call by using the shared hexToBytes utility: import { hexToBytes } from '@xrplf/isomorphic/utils' and replace "const buffer = Buffer.from(normalizedPrivateKey, 'hex')" with "const buffer = hexToBytes(normalizedPrivateKey)" (or if the rest of the code requires a Node Buffer, use Buffer.from(hexToBytes(normalizedPrivateKey))). This keeps browser-compatible behavior consistent with other modules and reuses the existing utility; update the import at the top of the module accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ripple-keypairs/src/signing-schemes/ed25519/index.ts`:
- Around line 22-41: deriveKeypairFromPrivateKey returns the original input
which can be a 64-char key without ED_PREFIX, causing sign() (which expects a
66-char prefixed key) to fail; update deriveKeypairFromPrivateKey to always
return the normalized private key including ED_PREFIX (use the existing
normalizedPrivateKey variable and prepend ED_PREFIX when building the returned
privateKey) so callers and sign() consistently receive a prefixed key; keep
generation of publicKey as ED_PREFIX +
bytesToHex(nobleEd25519.getPublicKey(Buffer.from(normalizedPrivateKey, 'hex'))).
---
Nitpick comments:
In `@packages/ripple-keypairs/src/index.ts`:
- Around line 98-109: derivePublicKey duplicates algorithm detection; replace
its use of getAlgorithmFromPrivateKey with the existing
getSigningScheme(privateKey) helper, then branch on the returned signing scheme
(e.g., 'ecdsa-secp256k1' vs 'ed25519') to call
secp256k1.deriveKeypairFromPrivateKey(...) or
ed25519.deriveKeypairFromPrivateKey(...) and return .publicKey; this reuses
getSigningScheme, removes duplicated mapping and makes the final throw
unreachable so it can be eliminated or reduced to a defensive assertion if
desired.
In `@packages/ripple-keypairs/src/signing-schemes/ed25519/index.ts`:
- Line 37: Replace the direct Buffer.from call by using the shared hexToBytes
utility: import { hexToBytes } from '@xrplf/isomorphic/utils' and replace "const
buffer = Buffer.from(normalizedPrivateKey, 'hex')" with "const buffer =
hexToBytes(normalizedPrivateKey)" (or if the rest of the code requires a Node
Buffer, use Buffer.from(hexToBytes(normalizedPrivateKey))). This keeps
browser-compatible behavior consistent with other modules and reuses the
existing utility; update the import at the top of the module accordingly.
In `@packages/xrpl/src/Wallet/index.ts`:
- Around line 177-190: Add an optional options parameter to fromPrivateKey to
accept a masterAddress for Regular Key Pair scenarios: change signature to
fromPrivateKey(privateKey: string, options?: { masterAddress?: string }):
Wallet, validate that options.masterAddress (if provided) is a non-empty string
(throw ValidationError otherwise), and pass it into the Wallet constructor
(i.e., new Wallet(derivePublicKey(privateKey), privateKey,
options?.masterAddress)) so behavior matches fromSeed/fromEntropy/fromMnemonic.
In `@packages/xrpl/test/wallet/index.test.ts`:
- Line 440: The test suite title is using "from PrivateKey" with a space but
should be consistent with other suites; update the describe block that currently
reads "from PrivateKey" to "fromPrivateKey" so it matches the method naming
style used by other tests (e.g., fromSeed, fromSecret, fromMnemonic,
fromEntropy) and ensures consistency with the tested function name
fromPrivateKey.
- Around line 442-448: Rename the snake_case test variables to camelCase (e.g.,
change mockWallet_secp256k1 -> mockWalletSecp256k1 and mockWallet_ed25519 ->
mockWalletEd25519) in the test file; update every reference to these identifiers
in tests (constructors, assertions, imports/exports within the file) so the new
names are used consistently, keeping the object shapes and values unchanged and
ensuring no leftover snake_case identifiers remain.
- Around line 440-483: Add a test under the "from PrivateKey" suite (near the
secp256k1/ed25519 tests) that calls Wallet.fromPrivateKey with an empty string
and asserts it throws the expected ValidationError; specifically, add an it
block that calls Wallet.fromPrivateKey('') and asserts the thrown error matches
/privateKey must be a non-empty string/ (reference: Wallet.fromPrivateKey and
the existing test cases in this describe block).
- Around line 456-460: The tests that call Wallet.fromPrivateKey with malformed
secp256k1 input should assert the specific error type or message rather than
only that something was thrown; update the failing assertions (the test invoking
Wallet.fromPrivateKey(mockWallet_secp256k1.privateKey.slice(0, 10)) and the
similar case around lines 477-481) to pass a second argument to assert.throws
that checks either an error constructor or a RegExp for the expected message
(e.g., assert.throws(() => Wallet.fromPrivateKey(...), { name: 'Error', message:
/malformed|invalid private key/i }) or assert.throws(() =>
Wallet.fromPrivateKey(...), /invalid private key/)) so the tests validate the
exact failure mode from Wallet.fromPrivateKey.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/ripple-keypairs/src/index.tspackages/ripple-keypairs/src/signing-schemes/ed25519/index.tspackages/ripple-keypairs/src/signing-schemes/secp256k1/index.tspackages/ripple-keypairs/src/types.tspackages/xrpl/src/Wallet/index.tspackages/xrpl/test/wallet/index.test.ts
| deriveKeypairFromPrivateKey(privateKey: string): { | ||
| privateKey: string | ||
| publicKey: string | ||
| } { | ||
| assert.ok( | ||
| privateKey.startsWith(ED_PREFIX) | ||
| ? privateKey.length === 66 | ||
| : privateKey.length === 64, | ||
| 'Invalid ed25519 private key length', | ||
| ) | ||
|
|
||
| const normalizedPrivateKey = privateKey.startsWith(ED_PREFIX) | ||
| ? privateKey.slice(2) | ||
| : privateKey | ||
|
|
||
| const buffer = Buffer.from(normalizedPrivateKey, 'hex') | ||
|
|
||
| const publicKey = ED_PREFIX + bytesToHex(nobleEd25519.getPublicKey(buffer)) | ||
| return { privateKey, publicKey } | ||
| }, |
There was a problem hiding this comment.
Inconsistent private key format returned may cause signing failures.
The method returns the original privateKey parameter (line 40), but the sign method (line 46) asserts that privateKey.length === 66 (including the ED prefix). If a user provides a 64-character private key without the prefix, the returned keypair's privateKey will fail validation when used with sign().
Consider normalizing the returned private key to always include the prefix:
🔧 Proposed fix
deriveKeypairFromPrivateKey(privateKey: string): {
privateKey: string
publicKey: string
} {
assert.ok(
privateKey.startsWith(ED_PREFIX)
? privateKey.length === 66
: privateKey.length === 64,
'Invalid ed25519 private key length',
)
const normalizedPrivateKey = privateKey.startsWith(ED_PREFIX)
? privateKey.slice(2)
: privateKey
const buffer = Buffer.from(normalizedPrivateKey, 'hex')
const publicKey = ED_PREFIX + bytesToHex(nobleEd25519.getPublicKey(buffer))
- return { privateKey, publicKey }
+ return { privateKey: ED_PREFIX + normalizedPrivateKey.toUpperCase(), publicKey }
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deriveKeypairFromPrivateKey(privateKey: string): { | |
| privateKey: string | |
| publicKey: string | |
| } { | |
| assert.ok( | |
| privateKey.startsWith(ED_PREFIX) | |
| ? privateKey.length === 66 | |
| : privateKey.length === 64, | |
| 'Invalid ed25519 private key length', | |
| ) | |
| const normalizedPrivateKey = privateKey.startsWith(ED_PREFIX) | |
| ? privateKey.slice(2) | |
| : privateKey | |
| const buffer = Buffer.from(normalizedPrivateKey, 'hex') | |
| const publicKey = ED_PREFIX + bytesToHex(nobleEd25519.getPublicKey(buffer)) | |
| return { privateKey, publicKey } | |
| }, | |
| deriveKeypairFromPrivateKey(privateKey: string): { | |
| privateKey: string | |
| publicKey: string | |
| } { | |
| assert.ok( | |
| privateKey.startsWith(ED_PREFIX) | |
| ? privateKey.length === 66 | |
| : privateKey.length === 64, | |
| 'Invalid ed25519 private key length', | |
| ) | |
| const normalizedPrivateKey = privateKey.startsWith(ED_PREFIX) | |
| ? privateKey.slice(2) | |
| : privateKey | |
| const buffer = Buffer.from(normalizedPrivateKey, 'hex') | |
| const publicKey = ED_PREFIX + bytesToHex(nobleEd25519.getPublicKey(buffer)) | |
| return { privateKey: ED_PREFIX + normalizedPrivateKey.toUpperCase(), publicKey } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ripple-keypairs/src/signing-schemes/ed25519/index.ts` around lines
22 - 41, deriveKeypairFromPrivateKey returns the original input which can be a
64-char key without ED_PREFIX, causing sign() (which expects a 66-char prefixed
key) to fail; update deriveKeypairFromPrivateKey to always return the normalized
private key including ED_PREFIX (use the existing normalizedPrivateKey variable
and prepend ED_PREFIX when building the returned privateKey) so callers and
sign() consistently receive a prefixed key; keep generation of publicKey as
ED_PREFIX +
bytesToHex(nobleEd25519.getPublicKey(Buffer.from(normalizedPrivateKey, 'hex'))).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/xrpl/src/Wallet/index.ts (1)
8-8:⚠️ Potential issue | 🔴 CriticalMissing import for
derivePublicKeycauses a build break.Line 175 uses
derivePublicKey(privateKey), butderivePublicKeyis not imported at line 8. This will fail TypeScript compilation.🔧 Proposed fix
-import { deriveAddress, deriveKeypair, generateSeed } from 'ripple-keypairs' +import { + deriveAddress, + deriveKeypair, + derivePublicKey, + generateSeed, +} from 'ripple-keypairs'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/Wallet/index.ts` at line 8, The build fails because derivePublicKey is used but not imported from 'ripple-keypairs'; update the import statement that currently brings in deriveAddress, deriveKeypair, generateSeed to also import derivePublicKey so calls to derivePublicKey(privateKey) compile; locate the import at the top of the Wallet module (near deriveAddress/deriveKeypair/generateSeed) and add derivePublicKey to that named import list, then run the TypeScript build to verify the error is resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/xrpl/src/Wallet/index.ts`:
- Line 8: The build fails because derivePublicKey is used but not imported from
'ripple-keypairs'; update the import statement that currently brings in
deriveAddress, deriveKeypair, generateSeed to also import derivePublicKey so
calls to derivePublicKey(privateKey) compile; locate the import at the top of
the Wallet module (near deriveAddress/deriveKeypair/generateSeed) and add
derivePublicKey to that named import list, then run the TypeScript build to
verify the error is resolved.
badb513 to
e397812
Compare
Add Joey Wallet (and others) to Application Docs
Added Joey Wallet as an available wallet option for developers. General details (and link to documentation) are provided.
Added similar information for other missing wallet.
Context of Change
This is a small change to the README files. It has no impact on the functionality of the packages herein.
Type of Change
Did you update HISTORY.md?
Test Plan